refactor(daemon): move four pure leaves to their kits - #2347
Conversation
`parameterized-recorded-fill.ts` has no value dependency on the daemon: it reads a `TargetAnnotationV1` type from contracts and calls `selectorContainsValue`, so its whole value graph already sits inside `@agent-device/selectors`. Move it there behind its own subpath and let the two daemon consumers reach it by specifier. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BPFjVXwbDrXPqp5W6K2iDK
`code-signature.ts` fingerprints a checkout from `node:crypto`/`fs`/`path` and `findProjectRoot`; `code-signature-cache.ts` adds a stat-validated cache over it through `publishFileSync`. Neither reaches the daemon, and both questions — what does this source tree hash to, and can that hash be replayed from stat alone — are host mechanics. Move both into host-kit behind their own subpaths, carrying `code-signature-cache.test.ts` unchanged apart from its specifiers, and let the launch spec and server lifecycle reach them by specifier. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BPFjVXwbDrXPqp5W6K2iDK
`screenshot-overlay.ts` decides which snapshot nodes earn a ref and where the ref lands on a screenshot; `screenshot-overlay-draw.ts` paints them. Both read kernel snapshot vocabulary, contracts snapshot predicates, and capture-kit's own PNG and rect-projection mechanics — nothing from the daemon. The two `src/snapshot/screenshot-overlay/` helpers had no other importer, and `react-native-overlay.ts` sits on kernel plus its contracts vocabulary alone. Move the cluster into capture-kit as flat siblings of the PNG and projection modules it already used, exposing `./screenshot-overlay` and `./react-native-overlay`; the draw, rects, and android halves stay package internals with no subpath of their own. The moved tests carry over unchanged apart from their specifiers, over a package-local snapshot-state fixture. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BPFjVXwbDrXPqp5W6K2iDK
`post-gesture-stability.ts` polls a caller-supplied snapshot function until a surface settles. It is generic over its snapshot and signature types and reads only host-kit diagnostics and `sleep`, so the loop is capture mechanics with no daemon knowledge; the daemon keeps the pending record, the comparator, and the verdict wiring it hands in. The verdict test stays in `src/daemon` because it composes the loop with the daemon's own `interaction-outcome-policy.ts`; only its specifier changes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BPFjVXwbDrXPqp5W6K2iDK
R11 pins every workspace package's exact subpath set, so the four moves need
their new specifiers named: `@agent-device/selectors/parameterized-recorded-fill`,
`@agent-device/host-kit/code-signature{,-cache}`, and
`@agent-device/capture-kit/{screenshot-overlay,react-native-overlay,post-gesture-stability}`.
The selectors comment counted its subpaths in prose; it now counts four and
says what the fourth is. No eager-closure row is needed: every new entry is a
rename the merge-base reader follows, and each closure is unchanged
(56/5/19/32/4/8), so all six fall under no-growth rather than the new-entry
ceiling.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BPFjVXwbDrXPqp5W6K2iDK
Size Report
Startup median (7 runs, lower is better):
|
The `fallow-ignore-next-line code-duplication` on the package-local snapshot-state fixture suppressed nothing: `fallow dupes` reports three clone groups on this tree and the fixture is in none of them, with or without the comment. A suppression that matches no finding is dead weight at best and a stale-suppression failure at worst. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BPFjVXwbDrXPqp5W6K2iDK
|
The moves are otherwise clean at c4f5633, but they break source-checkout daemon invalidation. walkDaemonCodeGraph follows only relative imports. The moved signature, overlay, fill and stability modules now sit behind workspace specifiers, so edits to them no longer change the running daemon signature. The cache also relies on the signature walker itself being in that graph, which is no longer true. Please fix workspace-export resolution at the signature owner, reusing the existing resolution machinery where possible. Add a regression proving that a package-owned runtime edit changes the source signature, including invalidation when the walker changes. Do not restore cross-package relative imports to work around it. |
thymikee
left a comment
There was a problem hiding this comment.
Adversarial review from the #2335 coordinator. I reviewed this branch at 41dec50 before the PR opened and raised one blocking finding directly with the author; recording the outcome here for the record, now confirmed at c4f5633.
The finding was: packages/capture-kit/src/snapshot-state.fixtures.ts duplicated makeSnapshotState byte-for-byte from src/__tests__/test-utils/snapshot-builders.ts:12 and carried // fallow-ignore-next-line code-duplication to silence the finding. AGENTS.md is explicit that gate failures are diagnosed at their reported invariant, not suppressed.
The resolution is right, and better than what I asked for. Rather than assume the suppression was load-bearing, the author measured: fallow dupes reports 3 clone groups on this tree and the fixture is in none, so the suppression was never doing anything. It is gone at c4f5633 and the gate passes without it. That is the correct answer — I had assumed the ignore was covering a real finding, and the measurement showed it was not.
The remaining duplication is a package-local test fixture, which is the shape this repo already uses (tmp-dir.fixtures.ts in capture-kit and host-kit) for the good reason that a package test cannot reach root src/__tests__. Pressing further would turn a rename-only move into a refactor of shared test utilities, against this issue's own non-goals. Settled.
Verified independently at c4f5633: all eight moved modules are true renames under -M90% whose content deltas are import-specifier rewrites only; four implementation commits plus one final chore(gates); session-target-evidence.ts correctly untouched; parameterized-recorded-fill.ts landed in @agent-device/selectors, avoiding the ad-script trap the issue flags; no re-export shim in src/daemon, src/snapshot or src/core. I independently confirmed both completion claims I would otherwise have taken on trust: SESSION_STATE_FIELD_OWNERS in scripts/layering/session-state.ts names none of the moved files, and no fallow-baselines/ entry is keyed to any moved path.
The consumer correction in the Summary is right and I have verified it: on main, sparse-fallback-screenshot.ts and generic-runtime-execution.ts do not import screenshot-overlay; screenshot-runtime.ts and android-snapshot-timeout-evidence.ts do. That is a defect in #2340's text, not in this PR. I will carry it to the umbrella, because #2342 hand-lists its seams the same way and will need the same treatment against the code rather than the issue.
Keeping exports additions with their move commits is the right call for bisectability and is now stated. CI is green across every completed check.
Generated by Claude Code
…ource `walkDaemonCodeGraph` followed relative specifiers only, so a source checkout's signature covered whatever the daemon still imported by relative path. That was already lossy and the leaf moves made it wrong: the walker itself, the overlay, the recorded-fill and the stability loop all left the graph, so editing them no longer changed the signature a client compares a running daemon against, and the cache's format guard lost the "the walk invalidates every document" property its comment rests on. Measured from `src/daemon.ts`: 619 modules on main with the walker stamped, 611 after the moves with it gone. Resolve a scoped specifier through the owning workspace package's `exports` map and walk into the file it names. The manifest is stamped, not merely probed, so an `exports` retarget invalidates without either endpoint changing; an uninstalled package is recorded as an absent path. Installed dependencies are still not followed — they change on install, not on edit — and the test is structural rather than a name pattern. The graph is now 1459 modules and ~112ms cold, which is what the stat-validated cache exists to absorb. Regression: five of the six new walker tests fail against the previous walker, including one that stamps the real daemon graph and asserts the walker is in it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BPFjVXwbDrXPqp5W6K2iDK
|
Confirmed and fixed in 266a1ee. Walking
So the cache comment claiming "
Cost: the cold walk goes 619→1459 modules, Regression:
Generated by Claude Code |
|
The workspace source-signature issue is resolved in 266a1ee. Package source edits and export retargets now invalidate the cached signature, and the regression tests cover both paths. No remaining code findings. The checks completed so far pass; remaining CI is still running. |
thymikee
left a comment
There was a problem hiding this comment.
Second review pass from the #2335 coordinator, on 266a1ee — the source-signature fix. This is the most consequential change in the whole wave and it deserves to be read as such rather than waved through as part of a move PR. Two things to settle before merge; the fix itself is right.
What it actually found. walkDaemonCodeGraph followed relative specifiers only, so the daemon's signature covered whatever the daemon still imported by relative path — 619 of its modules. That was already wrong on main, independent of this PR: more than half the daemon was invisible to the signature a client compares a running daemon against. The leaf moves pushed the walker, the overlay, the recorded-fill and the stability loop out of the relative graph, taking it to 611 and, worse, taking the walker itself out of the graph it walks, which is the exact property code-signature-cache.ts's format guard rests on. Stamping the manifest rather than probing it, so an exports retarget invalidates without either endpoint changing, is the right call and closes the subtlest hole. Five of six new walker tests failing against the old walker is real regression proof.
1. Say plainly that this exceeds the move's blast radius. Restoring parity would mean getting back to ~619-module coverage. You went to 1459 — you fixed a pre-existing defect, not just the regression the moves caused. I think that is the correct choice and should stay in this PR: splitting it would knowingly ship a branch whose signature is more blind than main's, and AGENTS.md says to repair at the owning construction path rather than route around it. But #2340 is a rename-only issue, and a reviewer scanning for renames will not expect a 202-line behavior change to code-signature.ts. State it in the Summary as a scope expansion with its reason, per the "note any expansion of scope" requirement — right now the body still reads as four pure moves.
2. The startup numbers in the size report predate this commit. The +0.2 ms --version / −1.8 ms --help figures were measured at c4f5633, before the walker changed. The walk is on the daemon-client hot path: every invocation stat-validates the cached graph, and that graph went from 619 to 1459 entries — roughly 2.4× the stat calls per command, plus whatever absentPaths grew to. The 112 ms figure you quote is the cold walk, which the cache absorbs; the number that matters for CLI latency is the cached revalidation cost at 266a1ee. Please post the refreshed startup medians from the size report on this head, and the absentPaths count before and after. If --version is unchanged, that settles it in one line.
Minor, no action needed: SCOPED_SPECIFIER_RE will match @scope/name inside prose, comments and fixture strings, and unlike a stray relative specifier each miss costs a durable absentPaths entry the cache re-stats forever. Restricting to the scoped shape bounds this sensibly and the comment explains why — I mention it only so the trade-off is on the record.
Sequencing consequence for the umbrella. #2348 (A) creates @agent-device/command-registry and moves 12 modules into it — the same class of edge this walker could not follow. Its own thread already refers to a "shared source-signature prerequisite from #2347", so this fix is now load-bearing for A as well. That is a dependency #2335 does not declare (it lists A and E as independent). Either this lands before A, or A carries the same defect. I am recording it on the umbrella and sequencing E ahead of A accordingly.
Generated by Claude Code
|
Takes #2347's move of the daemon code-signature walker into `@agent-device/host-kit` together with its workspace-specifier resolution, and drops this branch's own copy of that fix: one resolver, the one on main. The claim this branch owed keeps its own test beside the walker — the daemon source graph stamps `packages/command-registry/src/registry.ts`, `catalog.ts` and the manifest, measured at 1,461 modules, and touching the registry changes the signature. `src/cli/commands/router-types.ts`: #2349's `CommandProgressState` import alongside this branch's catalog specifier. A fixture string in the relocated CLI-route test also goes back to the specifier it illustrates; the move rewrote it as if it were a real import. Three review points, recorded here because their commits are pushed: - Dropping `registry.ts` and `platform-execution-entry.ts` from `HUB_ENTRY_FILES` is not only bookkeeping: the package manifest now publishes them, so the gate discovers them as domain facades and `denyPlatformImplementations` flips false to true. Both now carry the ADR-0019 assertion that they evaluate no concrete platform implementation, and both pass it. That strengthening is intended. - The 150 this branch reports against the issue's 154 is a filter difference, not a discrepancy: `pnpm depgraph` collapses to one edge per file pair, and 150 counts value edges only. Counting type-only and dynamic edges on the same graph gives 174 into 67 root files, which is the shape the issue's own per-file breakdown uses; both readings drop by exactly the 36 the issue predicts. - The six tests that stay in root do so for one root symbol each: parity needs STRUCTURED_BATCH_COMMAND_NAMES, DAEMON_COMMAND_DESCRIPTORS, canRunReplayScopedAction and DaemonRequest; ref-frame-effect needs DAEMON_COMMAND_DESCRIPTORS, resolveRefFrameEffect and DaemonRequest; timeout-policy needs DEFAULT_STABLE_TIMEOUT_MS; platform-execution-cli-route needs CLI_INJECTED_DAEMON_DISPATCHES and CliInjectedRoute; post-action-observation needs AgentDeviceClient, getCliCommandSchema, readInputFromCli, findCommandMetadata, the two command-family registry listers, SettleCapableClientOptionCommands, buildActionDetails and COMMAND_OUTPUT_SCHEMAS; shutdown-runtime-execution imports nothing but reads the daemon and root host sources whose text it pins. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016DkNZCMWtm1KUbseKANADi
Summary
Closes #2340. Four
src/daemonleaves move to the kit whose dependency set already covered them; each is a git rename carrying its tests.parameterized-recorded-fill.ts→@agent-device/selectors/parameterized-recorded-fillcode-signature.ts+code-signature-cache.ts→@agent-device/host-kit/code-signature{,-cache}screenshot-overlay{,-draw}.ts,snapshot/screenshot-overlay/{rects,android}.ts,core/react-native-overlay.ts→ capture-kit, exposing./screenshot-overlayand./react-native-overlay; draw/rects/android stay package internalspost-gesture-stability.ts→@agent-device/capture-kit/post-gesture-stabilitysession-target-evidence.tsis untouched and no re-export shim is left behind.One behaviour fix rides along, because the moves exposed it (review thread):
walkDaemonCodeGraphfollowed relative specifiers only, so moving daemon code behind workspace specifiers dropped it — and the walker itself — out of the source-checkout signature. It now resolves scoped specifiers through the owning package'sexportsmap, stamping the manifest so a retarget invalidates. Graph fromsrc/daemon.ts: 619 modules onmain, 611 after the moves alone, 1459 now.Issue correction: #2340 names
sparse-fallback-screenshot.tsandgeneric-runtime-execution.tsas overlay consumers, but neither imports it — the real consumers arescreenshot-runtime.tsandandroid-snapshot-timeout-evidence.ts.Package
exportsadditions sit in their move commits so each compiles and bisects; the R11 export-list pins are thechore(gates)commit.Validation
Full 58-check sweep at
41dec50; re-run at266a1ee: format, lint, typecheck, layering (186), eager-closure (454), fallow, production-exports and the signature/daemon-client suites (89) all green.git diff -M90% --stat origin/main...HEAD: every moved file a rename whose changed lines are specifier switches only.unit/vitest-relatedfail on 3 durable-capture tests that fail identically atorigin/main27a97ee and predate this branch; no open PR fixes them.Eager-closure green with no
APPROVED_OVER_CEILINGrow — rename detection carries each baseline, closures unchanged (56/5/19/32/4/8).R7 unaffected:
SESSION_STATE_FIELD_OWNERSinscripts/layering/session-state.tsnames none of the moved files.Fallow green; no baseline entry is keyed to a moved path, so none needed moving.
🤖 Generated with Claude Code
https://claude.ai/code/session_01BPFjVXwbDrXPqp5W6K2iDK